feat(storage): encrypt local sqlite databases - #2576

Claude finished @ethicnology's task in 9m 8s —— View job

Review: encrypted local SQLite databases

The overall design is strong — fail-closed key store with write-then-read-back verification, no API that can open the on-disk file without a key, verify-then-swap migration with atomic renames, keying before any other statement, and the key charset is both validated and escaped so the PRAGMA key interpolation is injection-safe. I found one high-severity data-loss window, one key-disclosure vector, and two lower-severity robustness gaps. Review was static (code reading); I did not run the test suites in this environment.

Note: posting inline review comments requires a gh api call that is not in this job's allowed tools, so all findings are consolidated here with permalinks. Add Bash(gh api:*) to --allowedTools if you want inline comments next time.

1. High — a stale background task can create an empty database mid-migration, and recovery then deletes the only surviving copies

handler.dart:42 · sqlite_database.dart:243-250

createIsolateWithSpawn opens NativeDatabase(File(dbPath)), which creates the file when it is missing, and drift then builds a fresh empty schema at the real path. Sequence:

1. The foreground crashes inside _encryptExistingDatabase between databaseFile.rename(backup.path) and temporary.rename(databaseFile.path). The key is already persisted, .plaintext-backup + .encryption-tmp hold all user data, and nothing exists at the real path. Workmanager().cancelAll() never ran on this launch (it runs after initLocator), so task schedules persisted by previous releases are still live.
2. A stale task fires. loadExisting() returns the key, so the guard in tasksHandler passes, and the isolate creates a brand-new empty but validly encrypted database at bullbitcoin_sqlite.sqlite.
3. Next foreground launch: _recoverEncryptionMigration takes the databaseExists && backupExists branch — the header is not plaintext and _verifyEncryptedDatabase passes on the empty database — so it deletes both .encryption-tmp and .plaintext-backup. Wallet metadata, labels, and transaction history are permanently lost.

Two cheap, complementary fixes:

• In the background LazyDatabase closure, bail out when the database file does not exist (a background task must never create it), and also when migration artifacts exist.
• In _recoverEncryptionMigration, when databaseFile, temporary and backup all exist, the database file cannot be the renamed temporary (the rename removes it) — treat it as foreign and restore from temporary/backup instead of deleting them.

Fix this →

2. Medium — the encryption key can leak into logs, Sentry, and the init error screen via SqliteException

sqlite_database.dart:162 · sqlite_database.dart:197 · payjoin_database.dart:104

package:sqlite3's SqliteException.toString() includes the causing statement. PRAGMA rekey rewrites the whole file and can genuinely fail midway (disk full, I/O error); PRAGMA key can fail on a corrupt/locked file. The raised exception then carries the literal PRAGMA rekey = '<key>' text and propagates out of Bull.init into log.severe('App Init Error', ...) (the on-disk TSV users are asked to share with support), AppInitErrorScreen(error: error) (rendered on screen), and Sentry via Report. Wrap the key/rekey statements in a try/catch that rethrows a sanitized StateError — same treatment in PayjoinDatabase.open.

Fix this →

3. Low — hasExistingDatabase ignores migration artifacts, so the never-mint-a-replacement-key invariant holds by ordering luck

main.dart:141-151

If a crash lands in the rename window (real file absent, backup + temp present) and the secure-storage key is genuinely gone — the exact case fail-closed exists for — loadOrCreate(hasExistingDatabase: false) mints a replacement key over existing data. Today recovery happens to save you: the temp fails verification under the new key and is deleted, then the plaintext backup is restored and re-encrypted. But that safety depends on the backup being plaintext and on recovery ordering, not on the invariant being enforced. Including .plaintext-backup/.encryption-tmp in the existence check makes it hold by construction.

4. Low — checkpoint result unchecked and sidecars deleted before the swap, so the rollback backup can silently miss WAL transactions

sqlite_database.dart:186

PRAGMA wal_checkpoint(TRUNCATE) reports "busy" in its result row rather than throwing, and execute discards that row. VACUUM INTO reads through the WAL so the temporary copy is complete either way — but the -wal file is deleted before the plaintext file becomes .plaintext-backup. If the checkpoint was busy (e.g. the stale background connection from finding 1 holds the file), any later rollback to the backup silently rewinds the un-checkpointed transactions. select the checkpoint result and fail/retry on busy, or delete the sidecars only after the swap succeeds.

Notes (no action required)

• Payjoin quarantine now also fires on key mismatch: any state that produces a different key silently discards payjoin sessions (payjoin_runtime.dart:50) instead of failing closed like the main database. Currently unreachable because the existence check covers payjoin.sqlite, but worth keeping in mind when the iOS keychain-lifecycle work (remaining items 1–3) lands.
• Keying uses passphrase mode, so sqlite3mc runs its KDF on every connection open even though the stored key is already 32 random bytes. Raw-key mode (PRAGMA key = "x'<hex>'") would skip that, but switching now would break already-migrated installs — fine as-is.
• Verified: bull_payjoin is a workspace member, so the root hooks.user_defines.sqlite3.source: sqlite3mc applies to its tests too; the remaining unkeyed SqliteDatabase(...) call sites are all in-memory/test executors; and integration_test/coins_test.dart opens its own temp file, not the production one.

Todo

• Gather context (CLAUDE.md, full diff vs origin/develop)
• Review key store, migration path, and database open paths
• Review background task / payjoin runtime changes
• Post review (consolidated here; inline comments not permitted by allowed tools)